Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Refactor SafeDogStatsdLogger to use get_validator to enable pattern matching #39370

Merged
merged 5 commits into from
May 6, 2024
Merged

Refactor SafeDogStatsdLogger to use get_validator to enable pattern matching #39370

merged 5 commits into from
May 6, 2024

Conversation

rawwar
Copy link
Collaborator

@rawwar rawwar commented May 2, 2024 •
edited
Loading

closes #39368

By reusing the get_validators function, we can incorporate pattern search to include allow/block lists.

@rawwar rawwar changed the title [feat] Add pattern matching to SafeDogStatsdLogger [feat] Refactor SafeDogStatsdLogger to use get_validator to get validators May 2, 2024
@rawwar rawwar marked this pull request as ready for review May 2, 2024 13:25
@rawwar rawwar changed the title [feat] Refactor SafeDogStatsdLogger to use get_validator to get validators Refactor SafeDogStatsdLogger to use get_validator to get validators May 2, 2024
@rawwar rawwar changed the title Refactor SafeDogStatsdLogger to use get_validator to get validators Refactor SafeDogStatsdLogger to use get_validator to enable pattern matching May 2, 2024
pankajkoti
pankajkoti reviewed May 2, 2024
View reviewed changes
Copy link
Member

@pankajkoti pankajkoti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

were you able to test this end to end with Datadog using all the 3 configurations? Allow list, block list and pattern list? Also could you please add tests for all those configurations if they don't exist already?

@rawwar
Copy link
Collaborator Author

rawwar commented May 3, 2024

@pankajkoti, Yes, I have tested them. Allow/Block list without pattern worked perfectly. However, I noticed the following behaviour when using pattern matching, which I am not sure if it's right.

With pattern matching enabled:

  1. AIRFLOW__METRICS__METRICS_ALLOW_LIST = .*queued_duration.* : Worked perfectly
image
  1. AIRFLOW__METRICS__METRICS_ALLOW_LIST = .*get_astronauts.* : Not sure if this is correct
image

In the 2nd screenshot, I still see several metrics that don't have get_astronauts anywhere in their metrics names. Is this correct behaviour? One of them is airflow.ti.finish

@rawwar
Copy link
Collaborator Author

rawwar commented May 3, 2024 •
edited
Loading

Block lists seem to be working as intended.

  1. AIRFLOW__METRICS__METRICS_BLOCK_LIST = .*duration.* : works perfectly
image

FYI: I have been using different tags for different settings to differentiate between searches.

@rawwar
Copy link
Collaborator Author

rawwar commented May 3, 2024 •
edited
Loading

Env variables set:

AIRFLOW__METRICS__STATSD_DATADOG_ENABLED true
AIRFLOW__METRICS__STATSD_DATADOG_TAGS testNo:C2
DATADOG_API_KEY ● ● ● ● ● ● ●
AIRFLOW__METRICS__METRICS_USE_PATTERN_MATCH true
AIRFLOW__METRICS__METRICS_ALLOW_LIST .*print_astronaut_craft.*

@pankajkoti pankajkoti requested a review from ferruzzi May 3, 2024 11:59
dirrao
dirrao approved these changes May 3, 2024
View reviewed changes
Copy link
Contributor

@dirrao dirrao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

ferruzzi
ferruzzi approved these changes May 3, 2024
View reviewed changes
Copy link
Contributor

@ferruzzi ferruzzi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code change LGTM, but I'm not sure that second allow-list example (the one you seemed unsure about) is working as expected. Either way, that is not in scope for this PR if that is broken, but you may want to look into that more if you have the time.

@rawwar
Copy link
Collaborator Author

rawwar commented May 4, 2024 •
edited
Loading

Code change LGTM, but I'm not sure that second allow-list example (the one you seemed unsure about) is working as expected. Either way, that is not in scope for this PR if that is broken, but you may want to look into that more if you have the time.

I will look into it. I will create an issue( #39400) and post my findings there.

@pankajkoti pankajkoti merged commit 0c4ffb8 into apache:main May 6, 2024
41 checks passed
@rawwar rawwar deleted the kalyan/datadog_add_pattern_match branch May 6, 2024 11:34
@pankajkoti pankajkoti added this to the Airflow 2.9.2 milestone May 6, 2024
@ephraimbuddy ephraimbuddy added the type:bug-fix Changelog: Bug Fixes label May 7, 2024
pateash pushed a commit to pateash/airflow that referenced this pull request May 13, 2024
@rawwar @pateash
Refactor SafeDogStatsdLogger to use get_validator to enable patte…
e9d1cdf 
…rn matching (apache#39370)

closes apache#39368

By reusing the get_validators function, we can incorporate pattern search to include allow/block lists.
ephraimbuddy pushed a commit that referenced this pull request Jun 4, 2024
@rawwar @ephraimbuddy
Refactor SafeDogStatsdLogger to use get_validator to enable patte…
2d88cca 
…rn matching (#39370)

closes #39368

By reusing the get_validators function, we can incorporate pattern search to include allow/block lists.

(cherry picked from commit 0c4ffb8)
@utkarsharma2 utkarsharma2 mentioned this pull request Jun 6, 2024
Closed
41 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dirrao dirrao dirrao approved these changes

@pankajkoti pankajkoti pankajkoti approved these changes

@ferruzzi ferruzzi ferruzzi approved these changes

Assignees
No one assigned
Labels
type:bug-fix Changelog: Bug Fixes
Projects
None yet
Milestone
Airflow 2.9.2
Development

Successfully merging this pull request may close these issues.

Add a pattern-matching support for allow and block list in Datadog metric
5 participants
@rawwar @ferruzzi @pankajkoti @dirrao @ephraimbuddy